feat: propagate errors up subgraphs and show slot errors in subgraphs #6963
Conversation
📝 WalkthroughWalkthroughRemoved inline nodeError stroke styling from the litegraph service and implemented a reactive error mapping and lookup system in the execution store with helpers to query node and slot errors and a watcher that marks nodes/slots and propagates errors up parent subgraphs. Changes
Sequence Diagram(s)sequenceDiagram
participant LN as lastNodeErrors (reactive)
participant W as Watch handler
participant ES as Execution Store
participant M as nodeErrorsByLocatorId (computed)
participant N as Nodes & Slots
participant P as Parent Subgraphs
LN->>W: emits updated errors
W->>N: clear all node & slot error flags
W->>ES: iterate new errors
ES->>M: map errors to NodeLocatorId
M->>N: mark nodes with errors
M->>N: mark input slots with errors
N->>P: propagate error flags up parent chain
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (7)**/*.{vue,ts,tsx}📄 CodeRabbit inference engine (.cursorrules)
Files:
**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursorrules)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursorrules)
Files:
**/*.{ts,tsx,js,vue}📄 CodeRabbit inference engine (.cursorrules)
Files:
**/*.{ts,tsx,js,jsx,vue}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{ts,tsx,vue}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.ts📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (8)📚 Learning: 2025-11-24T19:47:34.324ZApplied to files:
📚 Learning: 2025-11-24T19:46:52.279ZApplied to files:
📚 Learning: 2025-11-24T19:47:02.860ZApplied to files:
📚 Learning: 2025-11-24T19:48:23.088ZApplied to files:
📚 Learning: 2025-11-24T19:47:45.616ZApplied to files:
📚 Learning: 2025-11-24T19:46:52.279ZApplied to files:
📚 Learning: 2025-11-24T19:47:14.779ZApplied to files:
📚 Learning: 2025-11-24T19:48:23.088ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (1)
Comment |
dosubot
bot
added
the
size:L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/stores/executionStore.ts (1)
289-336: Error flag watcher is conceptually solid; verify theapp.graph.nodesguard and propagationThe watcher correctly resets all node/slot error flags, reapplies them from
lastNodeErrors, and propagateshas_errorsup the subgraph chain via sliced execution IDs, which matches the execution ID semantics. Two things to verify:
- The guard
if (!app.graph || !app.graph.nodes) returnassumes a.nodescollection on the graph, while other utilities tend to work off internal structures (e.g._nodesviaforEachNode). If the realLGraphdoesn’t expose.nodes, this watcher will never run in production. It may be safer to guard only onapp.graph(and letforEachNodehandle empty graphs).- For nested subgraphs, confirm via manual testing or an additional unit test that slicing
executionId.split(':')and walking parents (1..parts.length-1) lines up with howgetNodeByExecutionIdinterprets execution IDs in deeply nested graphs.If either of these assumptions doesn’t hold, you may need a small adjustment to the guard or an extra test around the watcher behavior.
tests-ui/tests/store/executionStore.test.ts (1)
109-276: Good coverage for node error lookups; avoid newas anyin mocksThe new tests do a nice job exercising
getNodeErrorsandslotHasErrorfor root vs subgraph nodes, multiple errors per slot, and missingextra_info, which gives good confidence in the new store helpers.One thing to align with the TS guidelines: the
mockNodein the subgraph test is declared withas any. You can avoidanyby typing it as a minimal structural type instead of casting toany, for example:- const mockNode = { - id: 123, - isSubgraphNode: () => true, - subgraph: mockSubgraph - } as any + const mockNode: { + id: number + isSubgraphNode: () => boolean + subgraph: typeof mockSubgraph + } = { + id: 123, + isSubgraphNode: () => true, + subgraph: mockSubgraph + }This keeps the test strongly typed without changing its behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/services/litegraphService.ts(0 hunks)src/stores/executionStore.ts(4 hunks)tests-ui/tests/store/executionStore.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/services/litegraphService.ts
🧰 Additional context used
📓 Path-based instructions (15)
**/*.{vue,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{vue,ts,tsx}: Leverage VueUse functions for performance-enhancing utilities
Use vue-i18n in Composition API for any string literals and place new translation entries in src/locales/en/main.json
Files:
src/stores/executionStore.tstests-ui/tests/store/executionStore.test.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursorrules)
Use es-toolkit for utility functions
Files:
src/stores/executionStore.tstests-ui/tests/store/executionStore.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
Use TypeScript for type safety
**/*.{ts,tsx}: Never useanytype - use proper TypeScript types
Never useas anytype assertions - fix the underlying type issue
Files:
src/stores/executionStore.tstests-ui/tests/store/executionStore.test.ts
**/*.{ts,tsx,js,vue}
📄 CodeRabbit inference engine (.cursorrules)
Implement proper error handling in components and services
**/*.{ts,tsx,js,vue}: Use 2-space indentation, single quotes, no semicolons, and maintain 80-character line width as configured in.prettierrc
Organize imports by sorting and grouping by plugin, and runpnpm formatbefore committing
Files:
src/stores/executionStore.tstests-ui/tests/store/executionStore.test.ts
src/**/*.{vue,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{vue,ts}: Leverage VueUse functions for performance-enhancing styles
Implement proper error handling
Use vue-i18n in composition API for any string literals. Place new translation entries in src/locales/en/main.json
Files:
src/stores/executionStore.ts
src/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.ts: Use es-toolkit for utility functions
Use TypeScript for type safety
Files:
src/stores/executionStore.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Use camelCase for variable and setting names in TypeScript/Vue files
Files:
src/stores/executionStore.tstests-ui/tests/store/executionStore.test.ts
**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,vue}: Useconst settingStore = useSettingStore()andsettingStore.get('Comfy.SomeSetting')to retrieve settings in TypeScript/Vue files
Useawait settingStore.set('Comfy.SomeSetting', newValue)to update settings in TypeScript/Vue files
Check server capabilities usingapi.serverSupportsFeature('feature_name')before using enhanced features
Useapi.getServerFeature('config_name', defaultValue)to retrieve server feature configurationEnforce ESLint rules for Vue + TypeScript including: no floating promises, no unused imports, and i18n raw text restrictions in templates
Files:
src/stores/executionStore.tstests-ui/tests/store/executionStore.test.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Define dynamic setting defaults using runtime context with functions in settings configuration
UsedefaultsByInstallVersionproperty for gradual feature rollout based on version in settings configuration
Files:
src/stores/executionStore.tstests-ui/tests/store/executionStore.test.ts
src/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/*.{ts,tsx,vue}: Sanitize HTML with DOMPurify to prevent XSS attacks
Avoid using @ts-expect-error; use proper TypeScript types instead
Use es-toolkit for utility functions instead of other utility libraries
Implement proper TypeScript types throughout the codebase
Files:
src/stores/executionStore.ts
src/**/stores/**/*.{ts,tsx}
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/stores/**/*.{ts,tsx}: Maintain clear public interfaces and restrict extension access in stores
Use TypeScript for type safety in state management stores
Files:
src/stores/executionStore.ts
src/**/*.{vue,ts,tsx}
📄 CodeRabbit inference engine (src/CLAUDE.md)
Follow Vue 3 composition API style guide
Files:
src/stores/executionStore.ts
**/stores/*Store.ts
📄 CodeRabbit inference engine (AGENTS.md)
Name Pinia stores with the
*Store.tssuffix
Files:
src/stores/executionStore.ts
tests-ui/**/*.test.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (tests-ui/CLAUDE.md)
tests-ui/**/*.test.{js,ts,jsx,tsx}: Write tests for new features
Follow existing test patterns in the codebase
Use existing test utilities rather than writing custom utilities
Mock external dependencies in tests
Always prefer vitest mock functions over writing verbose manual mocks
Files:
tests-ui/tests/store/executionStore.test.ts
**/*.{test,spec}.{ts,tsx,js}
📄 CodeRabbit inference engine (AGENTS.md)
Unit and component tests should be located in
tests-ui/or co-located with components assrc/components/**/*.{test,spec}.ts; E2E tests should be inbrowser_tests/
Files:
tests-ui/tests/store/executionStore.test.ts
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:34.324Z
Learning: Proper error propagation in service layer and state management
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:34.324Z
Learning: Implement proper error handling in stores
📚 Learning: 2025-11-24T19:47:45.616Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/components/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:45.616Z
Learning: Applies to src/components/**/*.vue : Use ref/reactive for state management in Vue 3 Composition API
Applied to files:
src/stores/executionStore.ts
📚 Learning: 2025-11-24T19:47:14.779Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:14.779Z
Learning: Applies to **/*.{ts,tsx,vue} : Use `const settingStore = useSettingStore()` and `settingStore.get('Comfy.SomeSetting')` to retrieve settings in TypeScript/Vue files
Applied to files:
src/stores/executionStore.ts
📚 Learning: 2025-11-24T19:47:45.616Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/components/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:45.616Z
Learning: Applies to src/components/**/*.vue : Implement computed() for derived state in Vue 3 Composition API
Applied to files:
src/stores/executionStore.ts
📚 Learning: 2025-11-24T19:47:02.860Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T19:47:02.860Z
Learning: Applies to src/**/*.vue : Utilize ref and reactive for reactive state
Applied to files:
src/stores/executionStore.ts
📚 Learning: 2025-11-24T19:46:52.279Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-24T19:46:52.279Z
Learning: Applies to **/*.vue : Utilize ref and reactive for reactive state in Vue 3
Applied to files:
src/stores/executionStore.ts
📚 Learning: 2025-11-24T19:47:56.371Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{test,spec}.{ts,tsx} : Use provided test helpers `createTestSubgraph` and `createTestSubgraphNode` from `./fixtures/subgraphHelpers` for consistent subgraph test setup
Applied to files:
tests-ui/tests/store/executionStore.test.ts
📚 Learning: 2025-11-24T19:48:03.270Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: tests-ui/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:48:03.270Z
Learning: Applies to tests-ui/**/*.test.{js,ts,jsx,tsx} : Write tests for new features
Applied to files:
tests-ui/tests/store/executionStore.test.ts
📚 Learning: 2025-11-24T19:47:56.371Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{test,spec}.{ts,tsx} : When writing tests for subgraph-related code, always import from the barrel export at `@/lib/litegraph/src/litegraph` to avoid circular dependency issues
Applied to files:
tests-ui/tests/store/executionStore.test.ts
📚 Learning: 2025-11-24T19:47:22.909Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Test user workflows in browser tests
Applied to files:
tests-ui/tests/store/executionStore.test.ts
📚 Learning: 2025-11-24T19:47:56.371Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{test,spec}.{js,ts,jsx,tsx} : When adding features, always write vitest unit tests using cursor rules in @.cursor
Applied to files:
tests-ui/tests/store/executionStore.test.ts
📚 Learning: 2025-11-24T19:47:34.324Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:34.324Z
Learning: Applies to src/**/stores/**/*.{ts,tsx} : Maintain clear public interfaces and restrict extension access in stores
Applied to files:
tests-ui/tests/store/executionStore.test.ts
🧬 Code graph analysis (1)
src/stores/executionStore.ts (4)
src/types/nodeIdentification.ts (1)
NodeLocatorId(18-18)src/types/index.ts (2)
NodeLocatorId(34-34)NodeError(48-48)src/scripts/app.ts (2)
lastNodeErrors(204-206)app(1678-1678)src/utils/graphTraversalUtil.ts (2)
forEachNode(156-169)getNodeByExecutionId(285-307)
🪛 ESLint
src/stores/executionStore.ts
[error] 2-2: Unable to resolve path to module 'vue'.
(import-x/no-unresolved)
[error] 35-35: Unable to resolve path to module '@/utils/graphTraversalUtil'.
(import-x/no-unresolved)
🔇 Additional comments (2)
src/stores/executionStore.ts (2)
247-283: Node error mapping and slot helper look correct; watch for potential ID collisionsThe
nodeErrorsByLocatorId,getNodeErrors, andslotHasErrorlogic is clean and matches the expectedNodeErrorshape, including safe handling of missingextra_info. One thing to double‑check: if multiple execution IDs can legitimately map to the sameNodeLocatorId(e.g. multiple instances of the same subgraph), this implementation will silently let the last one win in themap[locatorId] = nodeErrorassignment. If that scenario is possible, you may want to either aggregate errors or make that overwrite explicit in a comment so the behavior is intentional.
579-582: Public store API additions are cohesiveExposing
nodeLocatorIdToExecutionId,getNodeErrors, andslotHasErrorhere makes the execution store a clear single source of truth for execution↔locator conversions and error lookups, which fits the “clear public interfaces” guidance for stores. No issues from an API surface perspective.
| getNodeErrors, | ||
| slotHasError |
There was a problem hiding this comment.
Are these just exposed for the test?
If so, is there a way to test the behavior through the public API?
There was a problem hiding this comment.
DrJKL
left a comment
DrJKL
left a comment
There was a problem hiding this comment.
Testing internal logic piece can (and should) be fixed later.
Resolved conflicts by: - Keeping our node/slot error flag updates in executionStore - Removing duplicate nodeError stroke style (uses built-in has_errors) - Re-adding our error lookup tests - Accepting main's litegraphService refactor
🎭 Playwright Test Results❌ Some tests failed ⏰ Completed at: 12/03/2025, 10:18:15 PM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 12/03/2025, 10:06:04 PM UTC 🔗 Links🎉 Your Storybook is ready for review! |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 3.2 MB (baseline 3.2 MB) • 🔴 +1.94 kBMain entry bundles and manifests
Status: 3 added / 3 removed Graph Workspace — 972 kB (baseline 972 kB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 6.54 kB (baseline 6.54 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 1 added / 1 removed Panels & Settings — 298 kB (baseline 298 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 6 added / 6 removed UI Components — 169 kB (baseline 169 kB) • ⚪ 0 BReusable component library chunks
Status: 6 added / 6 removed Data & Services — 12.5 kB (baseline 12.5 kB) • ⚪ 0 BStores, services, APIs, and repositories
Status: 2 added / 2 removed Utilities & Hooks — 2.94 kB (baseline 2.94 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 1 added / 1 removed Vendor & Third-Party — 8.56 MB (baseline 8.56 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 3.81 MB (baseline 3.81 MB) • ⚪ 0 BBundles that do not match a named category
Status: 17 added / 17 removed |
2025-11-13.17-59-01_processed_20251113_175953.mp4
┆Issue is synchronized with this Notion page by Unito